[reference] commons-httpclient-2.0 blind regeneration for #11927#11935
[reference] commons-httpclient-2.0 blind regeneration for #11927#11935jordan-wong wants to merge 1 commit into
Conversation
This branch contains the output of an automated integration-authoring pass on `commons-httpclient-2.0`, produced against the draft skill in #11927. Opened as a draft PR for reviewer reference during discussion of that skill PR; not intended to be merged as-is. Base: master @ cd03735 Target module deleted before the run so the authoring flow had to generate it from scratch (blind test — output has no visibility of the shipping module). Diff scope: only `dd-java-agent/instrumentation/commons-httpclient-2.0/` plus one entry in `metadata/supported-configurations.json`. All other generation artifacts (`.analysis/**`, workflow state) are excluded.
|
🎯 Code Coverage (details) 🔗 Commit SHA: ce955af | Docs | Datadog PR Page | Give us feedback! |
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
| takesArgument( | ||
| 0, named("org.apache.commons.httpclient.HostConfiguration"))) | ||
| .and(takesArgument(1, named("org.apache.commons.httpclient.HttpMethod"))) | ||
| .and(takesArgument(2, named("org.apache.commons.httpclient.HttpState"))), |
There was a problem hiding this comment.
AI attempt is more prescriptive about the arguments - not just 3 args one of which has a specific type, but explicitly list the types of all 3 args.
Since the advice only reads the argument at position 1, that's the main one we need to ensure has the right type.
Benefit of relaxing the arg type matching: flexibility against future refactoring
Downside: could match more method overrides than we originally expected
Suggestion: might still want to mention that it's the type of arguments read using @Advice.Argument(n) that's critical.
| DECORATE.onRequest(span, httpMethod); | ||
|
|
||
| return scope; | ||
| } catch (BlockingException e) { |
There was a problem hiding this comment.
This is an AppSec specific concern - suggest you speak to AppSec about how to incorporate this into the skill
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void methodEnter(@Advice.Argument(1) final HttpMethod httpMethod) { | ||
| DECORATE.injectContext(currentContext(), httpMethod, SETTER); | ||
| DECORATE.injectContext(Context.current(), httpMethod, SETTER); |
There was a problem hiding this comment.
This could break downstream users because Context.current() is a static interface method. Although this is supported since Java 8, if this advice was patched into a pre-Java-8 class then the JVM would refuse to load it.
We need to add something like the following rule to the advice section:
Advice code must not contain calls to static interface methods, such as
Context.current(). Replace such calls with a statically imported method fromJava8BytecodeBridge.
2 participants
Reference draft for the skill PR #11927. This diff is the output of an automated integration-authoring pass on
commons-httpclient-2.0against the draft skill in that PR. Not intended to be merged as-is — opened so reviewers of #11927 can see concrete output the skill drives.What this is
Master's
commons-httpclient-2.0module was deleted, then the automated authoring flow was run against the draft skill (rules being proposed in #11927). This branch is the resulting output.Base: master @
cd03735159Scope: 9 files, +108/-352 — only inside
dd-java-agent/instrumentation/commons-httpclient-2.0/and one entry inmetadata/supported-configurations.json.Rule adherence — how the output compares to the shipping module
[2.0,)to[2.0,4.0); secondpassblock for the IAST integration name (commons-http-client-x) dropped entirely;latestDepTestImplementationrange semantics changed ('(2.0,20000000]'→'2.+')super(...)naming (single-name for single-module frameworks)super("commons-http-client", "commons-httpclient-2.0"), adding a spurious-2.0version alias that mints a phantomDD_TRACE_COMMONS_HTTPCLIENT_2_0_ENABLEDflag. Master usessuper("commons-http-client")alone.HelperMethodsanti-pattern absenttestImplementationversion matches declared min*Advice.javahelperClassNames()The two FAILs are both cases where the rule text exists in #11927 and the output violated it. This is useful signal for reviewers: it shows which parts of the skill draft are load-bearing vs which are being applied even without explicit rules.
Not for merging
This branch supersedes nothing on master and is not a replacement for the existing production module. The shipping
commons-httpclient-2.0is under separate open PRs (#11717) with its own review track.